Skip to content

Layer SCSS to MergeStyles#3979

Merged
oengusmacinog-zz merged 10 commits intomicrosoft:masterfrom
oengusmacinog-zz:layer-scss2ms
Feb 21, 2018
Merged

Layer SCSS to MergeStyles#3979
oengusmacinog-zz merged 10 commits intomicrosoft:masterfrom
oengusmacinog-zz:layer-scss2ms

Conversation

@oengusmacinog-zz
Copy link
Copy Markdown
Collaborator

@oengusmacinog-zz oengusmacinog-zz commented Feb 15, 2018

Pull request checklist

  • Include a change request file using $ npm run change
  • Delete comments left for review

Description of changes

This is the conversion of the Layer component to mergeStyles. The theme was left commented out due to the customizable decorator conflicting with static functions in the Component, but was unused in the styles to begin with. Also ContextualMenu had a test using the Layer Component now calling LaybeBase.

height: '100vh',
visibility: 'hidden'
}
],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing className at the end here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have a snapshot test, it would be great to update it and pass a className through so it shows up.


const getClassNames = classNamesFunction<ILayerStyleProps, ILayerStyles>();

// @customizable('Layer', ['theme'])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to uncomment this before checkin, maybe there is something we can do here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the old one didn't have it. I'm ok omitting it for now if we can't figure it out.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is commented out due to a bug, #3988. I'm leaving this commented out so it can be turned on later easily, but it's not blocking for this specific component because at least currently it does not need to use the theme.

@dzearing dzearing self-assigned this Feb 15, 2018
position: 'fixed',
zIndex: 1000000,
top: 0,
left: 0,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left get changed to right automatically by getClassNames when the page is RTL. I don't believe this should impact anything but please just verify that it works in RTL.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine in RTL mainly because the width is '100vw', so left:0 and right:0 are the same either way. The text automatically aligns to the right tho, but that's not part of this style set.

const { getStyles, theme } = this.props;
const classNames = getClassNames(getStyles!,
{
theme: theme!,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.props.className is never used, so that won't ever get added, even though it's in props and layerStyleProps

@oengusmacinog-zz oengusmacinog-zz merged commit 3cf8460 into microsoft:master Feb 21, 2018
@oengusmacinog-zz oengusmacinog-zz deleted the layer-scss2ms branch February 21, 2018 22:43
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants